Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfixes: initialize asg level to avoid randomized access rights and fix buffer overrun #52

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dirk-zimoch
Copy link

Access was sometimes not granted when it should be. Access changed randomly upon gateway restarts.
Generating an AS report showed random ASL values. Depending on it sign, access was granted or not.
This fix initializes the access level to 0.

@ralphlange
Copy link
Contributor

Duh. Thanks a lot!

Why is static analysis not finding this? It should.

@dirk-zimoch
Copy link
Author

I have not seen any compiler warnings about this either.

@mdavidsaver
Copy link

Maybe because lev is outside the loop?

Also, as I read it, lev is never set back to 0. So because this variable is outside the loop, won't this apply to all following lines? Has ASL0 ever worked?

@anjohnson
Copy link
Member

lev can be set to any int value by the sscanf() on line 609:

607:		if((asg=strtok(NULL," \t\n"))) {                           // ASG / ASL
608:			if((asl=strtok(NULL," \t\n")) &&
609:			  (sscanf(asl,"%d",&lev)!=1)) lev=1;
610:		} else {
611:			asg=(char*)default_group;
612:			lev=1;
613:		}

The inner if () only runs the lev=1 if the asl=strtok() fails or the sscanf() doesn't convert exactly one decimal integer.

Given that the default in the code above is ASL 1, I suspect this PR should be changed to set lev=1 instead of lev=0.

Dirk, can you look at the pvlist file where you were seeing this issue and confirm that the first PV name pattern in it gives an ASG name but not an ASL value? That's the only way I can see lev not being initialized by the existing code, although it is documentated that the ASL is optional in the pvlist's ALIAS and ALLOW lines.

@dirk-zimoch
Copy link
Author

None of my pvlist files use ASL. I usually use two ASGs, DEFAULT and WRITE, so my pvlist files have lines like
Pattern ALLOW
Pattern ALLOW WRITE

@anjohnson
Copy link
Member

Good, all our pvlist patterns that name an ASG also give an ASL, which explains why we haven't seen this problem here. We do make extensive use of ASL (or more accurately our pvlist files have patterns with both settings), but I don't know how important the choice is to our protection design.

I'm not certain that I understand how ASL is used in the gateway, but we do have ASGs with separate rules for level 0 and level 1 (not sure if higher level numers are allowed here or not, they aren't by the IOC) so this lets us set different access rules for different users on the same group or records.

@dirk-zimoch
Copy link
Author

docs/GATEWAY.pvlist also says the default is 1:

#        <asl>             = Access Security Level (0 or 1)                  [1]

I will change it.

Copy link
Member

@anjohnson anjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now. I won't click the "Approve and Run" button since the tests here failed last time I did that.

@mdavidsaver
Copy link

Initializing to 1 is a improvement. I think it should be done inside the loop though.

	while(fgets(inbuf,sizeof(inbuf),fd)) {
		++line;
		pattern=rname=hname=NULL;
                lev = 1; /* <-- add */

imo. a better long term strategy would be to move these variable definitions into the loop. I think this would more clearly represent the intent to readers, and to static analysis tools.

@dirk-zimoch dirk-zimoch changed the title bugfix: initialize asg level to avoid randomized access rights bugfixes: initialize asg level to avoid randomized access rights and fix buffer overrun Jun 3, 2024
@@ -391,7 +391,7 @@ gateAsEntry* gateAs::findEntryInList(const char* pv, gateAsList& list) const
int len = (int) strlen(pv);
#ifdef USE_PCRE
pi->substrings=pcre_exec(pi->pat_buff, NULL,
pv, len, 0, PCRE_ANCHORED, pi->ovector, 30);
pv, len, 0, PCRE_ANCHORED, pi->ovector, pi->ovecsize);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy-and-paste bug I had introduced in or before 2007.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants